fix(cli): keep overlap waivers local to marked text - #3464
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Follow-up on the same scene-4 case, after tracing why The caption gate runs only at the declared seeks, not at the merged layout grid. Those 10 times are a fixed fraction list, so spacing scales with duration. Consequence on the exact defect that motivated this PR. The scene-4 card is visible 2.86s. A 2.86s window against a 5.3s grid intersects with probability 2.86 / 5.3 ≈ 54%. So the gate catches the case it was built for a bit better than a coin flip, and gets worse as films get longer: at 90s the spacing is 9s and the same defect drops to ~32%. The good news, since it is easy to assume otherwise: Two ways to close it, both cheap:
(1) is the one I would take, since it stays correct as film length changes and (2) has to be re-tuned every time the format does. Verified at — Rames |
|
The 25-point grid checks out: One edge worth closing while you're in here: the 60s cap is prose, not an invariant. That matters because the guarantee is duration-dependent in exactly the way the ten-point grid was, just with a friendlier constant. At 75s the gap is 3.0s and the original 2.86s case silently goes back to being a coin flip — and a contract test that pins "25 points" plus "≤2.4s max gap" computed at a fixed duration would still be green while it happened. Two ways to make the test mean what it says:
Either is fine. The thing I'd avoid is having the only thing standing between the grid and under-sampling be a sentence in a docs budget. — Rames |
jrusso1020
left a comment
There was a problem hiding this comment.
Approved. Re-verified at head dbad0749a.
The change is right. hasAllowOverlapFlag moving from closest() to hasAttribute() has exactly one call site, isSolidTextBlock (layout-audit.browser.js:582), so the blast radius is contained to which elements get collected as overlap participants. The comment states the rationale accurately, and tightening is the correct direction for an audit tool.
The regression witness is non-vacuous. With rootAttrs: "data-layout-allow-overlap", the two descendants (100-500 x 100-200 and 300-700 x 120-220) genuinely overlap. Under the old closest() both inherited the root waiver, isSolidTextBlock returned false for each, and zero blocks were collected, so expect(...).toBe(true) fails pre-fix. The installOverlapStyles / installOverlapGeometry extraction preserves the original call order (styles, geometry, installAuditScript, runAudit), so it is behavior-neutral.
data-layout-allow-caption-zone is actually wired, not inert: layout-audit.browser.js:109 reads it via closest(), so the #cap-line assertion in changelogSkillContent.test.ts pins a real exemption rather than a name nothing consumes.
One non-blocking follow-up. The waiver semantics changed globally, but only the changelog-video skill was swept. Of the 38 in-repo uses of data-layout-allow-overlap, every one marks the leaf text element except registry/blocks/message-thread-reveal/message-thread-reveal.html:910, where #mtr-ec-wrap carries it as a container. That wrapper has text descendants (#mtr-ec-fam, three .ec-label pairs, #mtr-ec-cta) which inherited the waiver before and no longer do. They sit at distinct absolute left offsets so they may well not collide, and nothing in CI audits that block, but it is the one site where this change alters behavior outside the files you touched.
CI at this head is green across the board, with Tests on windows-latest still running.
— Rames
What
data-layout-allow-overlapapply only to the marked text block.Why
The overlap waiver used
closest(), so placing it on a mock-slide or composition wrapper silently disabled collision detection for every descendant. The changelog skill explicitly prescribed that broad marker. Separately, the original card only breached the canvas by 8px—below the generic painted-panel floor—so the changelog gate needs its known caption-rail band enabled rather than relying on generic overflow detection.Together these gaps allowed
hyperframes checkto report a false-clean layout while unrelated text collided or scene content entered the caption rail.How
The browser overlap audit now checks
hasAttribute()on each solid text block. Intentional layering remains waivable on the exact participant, while parent/root markers no longer exempt the subtree.The changelog scaffold marks
#cap-linewithdata-layout-allow-caption-zone, removes the slide-root overlap waiver, and its required check command samples the reserved bottom 10% at 25 evenly distributed timeline points (maximum 2.4s gap at the 60s skill duration cap).Test plan
bun run --cwd packages/cli typecheckbun run buildbun run lintbun run format:checkSeparate from the preview/seam-gate fix in #3463.